Skip to content

[SEMVER-MAJOR] Fix remoting strong-error-handler - #2375

Merged
davidcheung merged 1 commit into
masterfrom
fix-remoting-strongerrorhandler
Jun 7, 2016
Merged

[SEMVER-MAJOR] Fix remoting strong-error-handler#2375
davidcheung merged 1 commit into
masterfrom
fix-remoting-strongerrorhandler

Conversation

@davidcheung

@davidcheung davidcheung commented May 24, 2016

Copy link
Copy Markdown
Contributor

connect to https://github.com/strongloop-internal/scrum-loopback/issues/852

addresses test cases failing and stacktrace showing up on npm test

should land with strongloop/strong-remoting#302

@davidcheung

Copy link
Copy Markdown
Contributor Author

@bajtos PTAL

Comment thread test/app.test.js Outdated
var app, db;
beforeEach(function() {
app = loopback();
app.set('remoting', { errorHandler: { debug: false, log: false }});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to set debug:true in tests, so that we get more details in error messages. Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, not sure what i was thinking

@bajtos

bajtos commented Jun 1, 2016

Copy link
Copy Markdown
Member

@davidcheung the patch looks mostly good, my main concern is the inconsistency in debug setting across tests - most tests have debug:true, few have debug:false. Can we unify it and always set debug:true?

@bajtos bajtos assigned davidcheung and unassigned bajtos Jun 1, 2016
@davidcheung davidcheung assigned bajtos and unassigned davidcheung Jun 3, 2016
@davidcheung

Copy link
Copy Markdown
Contributor Author

@bajtos PTAL again please

@bajtos

bajtos commented Jun 3, 2016

Copy link
Copy Markdown
Member

@davidcheung the code changes LGTM, but the CI is all red. Please investigate. If it's something trivial, then feel free to land this without further reviews.

Also, could you please capture somewhere in release notes that by default, LoopBack 3.0-powered applications will log failed requests to stderr? We didn't log errors in 2.x, I think it may surprise people when they discover for the first time. The section in release notes should include instructions how to disable these log messages (revert back to 2.x behavior).

@bajtos bajtos assigned davidcheung and unassigned bajtos Jun 3, 2016
@davidcheung

Copy link
Copy Markdown
Contributor Author

@bajtos i will retrigger ci once strong-remoting lands, which should pass all the tests, the change in this PR is disabling stacktrace on new rest-adapter's error-handling, and removing the current ones

@davidcheung
davidcheung force-pushed the fix-remoting-strongerrorhandler branch from c1a1088 to d8113c8 Compare June 7, 2016 17:24
Fix rest-adapter related test case switching to strong-error-handler
Only affect the test-cases calling rest methods
@davidcheung
davidcheung force-pushed the fix-remoting-strongerrorhandler branch from d8113c8 to ddb5327 Compare June 7, 2016 17:32
@Amir-61 Amir-61 mentioned this pull request Jun 7, 2016
@rmg

rmg commented Jun 7, 2016

Copy link
Copy Markdown
Member

@slnode test please

@davidcheung
davidcheung merged commit ed45358 into master Jun 7, 2016
@davidcheung
davidcheung deleted the fix-remoting-strongerrorhandler branch June 7, 2016 21:36
@davidcheung davidcheung removed the #wip label Jun 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants